Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds native image upload functionality to the desktop UI, allowing users to directly attach images via a file picker button in addition to the existing paste/drag-and-drop methods. Images are automatically compressed to a maximum of 1024px on the longest side at 0.85 JPEG quality before being sent.
Changes:
- Extended the message type system to support image content with base64 data and MIME types
- Added image compression logic that resizes and converts images to JPEG format
- Implemented file input-based image selection with the same compression pipeline as paste functionality
- Updated the chat submission flow to pass image data directly in message content
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| ui/desktop/src/types/message.ts | Adds ImageData interface and updates createUserMessage to accept optional image data, building message content array with both text and images |
| ui/desktop/src/hooks/useChatStream.ts | Updates handleSubmit signature to accept optional ImageData array parameter |
| ui/desktop/src/components/ChatInput.tsx | Adds compressImageDataUrl function, implements file input handler for image uploads, extracts base64 image data for direct message content, and updates tooltip text |
| ui/desktop/src/components/BaseChat.tsx | Extracts image data from form submission event and passes to handleSubmit |
| } else { | ||
| setPastedImages(prev => prev.map(img => | ||
| img.id === uniqueId | ||
| ? { ...img, dataUrl: compressedDataUrl, isLoading: false, error: undefined } |
There was a problem hiding this comment.
Missing filePath assignment when save succeeds. The success path should include filePath: saveResult.filePath to be consistent with the paste handling logic (line 775) and to properly populate the PastedImage type's optional filePath field.
| ? { ...img, dataUrl: compressedDataUrl, isLoading: false, error: undefined } | |
| ? { ...img, dataUrl: compressedDataUrl, filePath: saveResult.filePath, isLoading: false, error: undefined } |
| // Convert to JPEG with 0.85 quality | ||
| const compressedDataUrl = canvas.toDataURL('image/jpeg', 0.85); | ||
| resolve(compressedDataUrl); |
There was a problem hiding this comment.
The image compression always converts to JPEG format regardless of the original format. This means transparent PNGs will lose their transparency (converted to black or white background). Consider preserving PNG format for images that support transparency, or at least documenting this behavior change for users.
| </Button> | ||
| </TooltipTrigger> | ||
| <TooltipContent>Attach file or directory</TooltipContent> | ||
| <TooltipContent>Attach file</TooltipContent> |
There was a problem hiding this comment.
The tooltip text changed from "Attach file or directory" to "Attach file". This change aligns with the new implementation which uses a file input that doesn't support directory selection. However, verify this is intentional - if directory selection is still needed, the implementation should be updated to support it.
| interface ImagePreviewProps { | ||
| src: string; | ||
| alt?: string; | ||
| className?: string; | ||
| } |
There was a problem hiding this comment.
The alt and className props were removed from the ImagePreview component interface but the component now always uses a hardcoded "goose image" alt text. For accessibility, consider keeping the alt prop to allow meaningful descriptions of each image, especially for screen reader users.
ui/desktop/src/types/message.ts
Outdated
| export function createUserMessage(text: string, images?: ImageData[]): Message { | ||
| const content: Message['content'] = []; | ||
|
|
||
| // Add text content if present | ||
| if (text.trim()) { | ||
| content.push({ type: 'text', text }); | ||
| } | ||
|
|
||
| // Add image content if present | ||
| if (images && images.length > 0) { | ||
| images.forEach(img => { | ||
| content.push({ | ||
| type: 'image', | ||
| data: img.data, | ||
| mimeType: img.mimeType, | ||
| }); | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
When creating a message with only images and no text, the content array will only contain image items. Verify the backend properly handles messages with no text content, as this is a new scenario enabled by this PR.
| interface PastedImage { | ||
| id: string; |
There was a problem hiding this comment.
The filePath property is no longer used since images are now handled as base64 data directly. This property should be removed from the interface. The old handlers that reference filePath (handleRetryImageSave and handleRemovePastedImage) also need to be updated or removed.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| const interruptionMessage: QueuedMessage = { | ||
| id: Date.now().toString() + Math.random().toString(36).substr(2, 9), | ||
| content: contentToQueue, | ||
| timestamp: Date.now(), | ||
| images: [], | ||
| }; |
There was a problem hiding this comment.
Pasted images are being lost when messages are queued during interruptions. The images array should be populated with the current pastedImages state (converted to ImageData format) rather than an empty array, similar to how it's done in the performSubmit function.
| const newMessage: QueuedMessage = { | ||
| id: Date.now().toString() + Math.random().toString(36).substr(2, 9), | ||
| content: contentToQueue, | ||
| timestamp: Date.now(), | ||
| images: [], | ||
| }; |
There was a problem hiding this comment.
Pasted images are being lost when messages are queued while loading. The images array should be populated with the current pastedImages state (converted to ImageData format) rather than an empty array, similar to how it's done in the performSubmit function.
| reader.onload = async (evt) => { | ||
| const dataUrl = evt.target?.result as string; | ||
| if (dataUrl) { | ||
| const compressedDataUrl = await compressImageDataUrl(dataUrl); | ||
| setPastedImages((prev) => | ||
| prev.map((img) => | ||
| img.id === uniqueId | ||
| ? { ...img, dataUrl: compressedDataUrl, isLoading: false, error: undefined } | ||
| : img | ||
| ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Missing error handling for image compression. If compressImageDataUrl throws an error (e.g., unsupported image format, corrupted data), it will result in an unhandled promise rejection. Add a try-catch block similar to the paste handling and other compression calls.
| reader.onload = async (e) => { | ||
| const dataUrl = e.target?.result as string; | ||
| if (dataUrl) { | ||
| // Update the image with the data URL | ||
| const compressedDataUrl = await compressImageDataUrl(dataUrl); | ||
| setPastedImages((prev) => | ||
| prev.map((img) => (img.id === imageId ? { ...img, dataUrl, isLoading: true } : img)) | ||
| prev.map((img) => | ||
| img.id === imageId ? { ...img, dataUrl: compressedDataUrl, isLoading: false } : img | ||
| ) | ||
| ); | ||
|
|
||
| try { | ||
| const result = await window.electron.saveDataUrlToTemp(dataUrl, imageId); | ||
| setPastedImages((prev) => | ||
| prev.map((img) => | ||
| img.id === result.id | ||
| ? { ...img, filePath: result.filePath, error: result.error, isLoading: false } | ||
| : img | ||
| ) | ||
| ); | ||
| } catch (err) { | ||
| console.error('Error saving pasted image:', err); | ||
| setPastedImages((prev) => | ||
| prev.map((img) => | ||
| img.id === imageId | ||
| ? { ...img, error: 'Failed to save image via Electron.', isLoading: false } | ||
| : img | ||
| ) | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing error handling for image compression. If compressImageDataUrl throws an error, it will result in an unhandled promise rejection. Wrap the compression call in a try-catch block to handle failures gracefully, similar to the error handling in useFileDrop.ts lines 81-100.
| const customEvent = e as unknown as CustomEvent; | ||
| const textValue = customEvent.detail?.value || ''; | ||
|
|
||
| const chatInputSubmit = (textValue: string, images: ImageData[]) => { |
There was a problem hiding this comment.
Missing import for ImageData. The function chatInputSubmit uses ImageData[] but the type is not imported. Add ImageData to the imports from '../types/message' on line 35.
| const compressedDataUrl = await compressImageDataUrl(dataUrl); | ||
| setPastedImages((prev) => | ||
| prev.map((img) => | ||
| img.id === uniqueId | ||
| ? { ...img, dataUrl: compressedDataUrl, isLoading: false, error: undefined } | ||
| : img | ||
| ) | ||
| ); |
There was a problem hiding this comment.
Missing error handling for compressImageDataUrl. If compression fails, the promise will reject but there's no try-catch block to handle it, leaving the image in a loading state indefinitely. Wrap the compressImageDataUrl call in a try-catch and update the image state with an error message on failure.
| const compressedDataUrl = await compressImageDataUrl(dataUrl); | |
| setPastedImages((prev) => | |
| prev.map((img) => | |
| img.id === uniqueId | |
| ? { ...img, dataUrl: compressedDataUrl, isLoading: false, error: undefined } | |
| : img | |
| ) | |
| ); | |
| try { | |
| const compressedDataUrl = await compressImageDataUrl(dataUrl); | |
| setPastedImages((prev) => | |
| prev.map((img) => | |
| img.id === uniqueId | |
| ? { ...img, dataUrl: compressedDataUrl, isLoading: false, error: undefined } | |
| : img | |
| ) | |
| ); | |
| } catch (error) { | |
| console.error('Failed to compress image data URL', error); | |
| setPastedImages((prev) => | |
| prev.map((img) => | |
| img.id === uniqueId | |
| ? { ...img, isLoading: false, error: 'Failed to process image file' } | |
| : img | |
| ) | |
| ); | |
| } |
| ); | ||
| try { | ||
| // Compress the image | ||
| const compressedDataUrl = await compressImageDataUrl(dataUrl); |
There was a problem hiding this comment.
Missing error handling for compressImageDataUrl. If compression fails, the promise will reject but there's no try-catch block to handle it, leaving the image in a loading state indefinitely. Wrap the compressImageDataUrl call in a try-catch and update the image state with an error message on failure.
| const dataUrl = e.target?.result as string; | ||
| if (dataUrl) { | ||
| // Update the image with the data URL | ||
| const compressedDataUrl = await compressImageDataUrl(dataUrl); |
There was a problem hiding this comment.
Missing error handling for compressImageDataUrl. If compression fails, the promise will reject but there's no try-catch block to handle it, leaving the image in a loading state indefinitely. Wrap the compressImageDataUrl call in a try-catch and update the image state with an error message on failure.
zanesq
left a comment
There was a problem hiding this comment.
Nice good improvement! Code LGTM and verified its working as intended uploading and processing images in chat locally.
Co-authored-by: Douwe Osinga <douwe@squareup.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: fbalicchia <fbalicchia@cuebiq.com>
* origin/main: Fix GCP Vertex AI global endpoint support for Gemini 3 models (#6187) fix: macOS keychain infinite prompt loop (#6620) chore: reduce duplicate or unused cargo deps (#6630) feat: codex subscription support (#6600) smoke test allow pass for flaky providers (#6638) feat: Add built-in skill for goose documentation reference (#6534) Native images (#6619) docs: ml-based prompt injection detection (#6627) Strip the audience for compacting (#6646) chore(release): release version 1.21.0 (minor) (#6634) add collapsable chat nav (#6649) fix: capitalize Rust in CONTRIBUTING.md (#6640) chore(deps): bump lodash from 4.17.21 to 4.17.23 in /ui/desktop (#6623) Vibe mcp apps (#6569) Add session forking capability (#5882) chore(deps): bump lodash from 4.17.21 to 4.17.23 in /documentation (#6624) fix(docs): use named import for globby v13 (#6639) PR Code Review (#6043) fix(docs): use dynamic import for globby ESM module (#6636) # Conflicts: # Cargo.lock # crates/goose-server/src/routes/session.rs
…o dkatz/canonical-context * 'dkatz/canonical-provider' of github.com:block/goose: (27 commits) docs: add Remotion video creation tutorial (#6675) docs: export recipe and copy yaml (#6680) Test against fastmcp (#6666) docs: mid-session changes (#6672) Fix MCP elicitation deadlock and improve UX (#6650) chore: upgrade to rmcp 0.14.0 (#6674) [docs] add MCP-UI to MCP Apps blog (#6664) ACP get working dir from args.cwd (#6653) Optimise load config in UI (#6662) Fix GCP Vertex AI global endpoint support for Gemini 3 models (#6187) fix: macOS keychain infinite prompt loop (#6620) chore: reduce duplicate or unused cargo deps (#6630) feat: codex subscription support (#6600) smoke test allow pass for flaky providers (#6638) feat: Add built-in skill for goose documentation reference (#6534) Native images (#6619) docs: ml-based prompt injection detection (#6627) Strip the audience for compacting (#6646) chore(release): release version 1.21.0 (minor) (#6634) add collapsable chat nav (#6649) ...
Summary
Upload images directly. Also does a better job on pasting images in: